PM-14175 - Replace vault illustration on create account screen #1102
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1102 +/- ##
==========================================
- Coverage 89.37% 89.37% -0.01%
==========================================
Files 677 677
Lines 42862 42883 +21
==========================================
+ Hits 38307 38325 +18
- Misses 4555 4558 +3 ☔ View full report in Codecov by Sentry. |
phil-livefront
requested review from
matt-livefront,
victor-livefront,
brant-livefront and
a team
as code owners
| private func mainContent(with proxy: GeometryProxy) -> some View { | ||
| ScrollView(showsIndicators: false) { | ||
| VStack(spacing: 0) { | ||
| if store.state.isCreateAccountFeatureFlagEnabled { | ||
| Spacer() | ||
|
|
||
| Image(decorative: Asset.Images.logo) | ||
| .resizable() | ||
| .scaledToFit() | ||
| .foregroundColor(Asset.Colors.iconSecondary.swiftUIColor) | ||
| .frame(maxWidth: .infinity, maxHeight: 34) | ||
| .padding(.horizontal, 12) | ||
|
|
||
| Spacer() | ||
| } | ||
|
|
||
| registrationDetails | ||
| } | ||
| .padding([.horizontal, .vertical], 16) | ||
| .frame(minHeight: store.state.isCreateAccountFeatureFlagEnabled ? proxy.size.height : 0) | ||
| } | ||
| .frame(width: proxy.size.width) | ||
| } |
There was a problem hiding this comment.
🎨 I might be tempted to include the GeometryReader in this function rather than pass it it. Alternatively, keep the GeometryReader in the body, and have this return the content that should be in the scroll view. It's minor, but I think it helps to keep the geometry reader closer to where it's used when possible. You could also use the scrollView() modifier to reduce one level of indentation and get the padding and background color as free defaults.
I also wonder if we should specify some minimum padding around the image for when the content grows.
GeometryReader { proxy in
mainContent()
.padding(.vertical, 16)
.frame(minHeight: store.state.isCreateAccountFeatureFlagEnabled ? proxy.size.height : 0)
.scrollView(addVerticalPadding: false)
}
private func mainContent() -> some View {
VStack(spacing: 0) {
if store.state.isCreateAccountFeatureFlagEnabled {
Spacer()
Image(decorative: Asset.Images.logo)
.resizable()
.scaledToFit()
.foregroundColor(Asset.Colors.iconSecondary.swiftUIColor)
.frame(maxWidth: .infinity, maxHeight: 34)
.padding(.horizontal, 12)
Spacer()
}
registrationDetails
}
}
…create-account-screen
| // MARK: View | ||
|
|
||
| func body(content: Content) -> some View { | ||
| ScrollView { | ||
| ScrollView(showsIndicators: showsIndicators) { |
There was a problem hiding this comment.
@matt-livefront this was also added in the other PR for the LandingView. Which over goes in first ill fix any conflicts that arise. I just didnt want to forge this on both.
There was a problem hiding this comment.
also not sure why this isn't being picked up by tests... do you see anything i did wrong herE?
There was a problem hiding this comment.
What's the reasoning behind hiding the scroll indicators?
There was a problem hiding this comment.
ah i should have explained that, my bad. I ran this by Emily and was asked to hide them.
…create-account-screen
| @@ -118,11 +118,13 @@ extension View { | |||
| /// | |||
| func scrollView( | |||
| addVerticalPadding: Bool = true, | |||
| backgroundColor: Color = Asset.Colors.backgroundPrimary.swiftUIColor | |||
| backgroundColor: Color = Asset.Colors.backgroundPrimary.swiftUIColor, | |||
| showsIndicators: Bool = true | |||
There was a problem hiding this comment.
🎨 Can you add this to the docs above?
| .frame(minHeight: store.state.isCreateAccountFeatureFlagEnabled ? proxy.size.height : 0) | ||
| .scrollView(showsIndicators: false) |
There was a problem hiding this comment.
⛏️ I think if we fix the frame to the height of the view, we need to remove the padding that scrollView adds and manually add it before the frame modifier. Otherwise, the content size is larger than the scroll bounds and it will scroll slightly.
There was a problem hiding this comment.
Did i fix this right? my eyes are being deceiving right now... the min spacer height i think is messing with me. Did i add a double top padding here?
🎟️ Tracking
PM-14175
📔 Objective
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes